Clean up the support for split queues #78274
Conversation
github-actions
bot
added
the
Scope: Backend
src/sentry/conf/server.py
Outdated
| @@ -821,6 +822,15 @@ def SOCIAL_AUTH_DEFAULT_USERNAME() -> str: | |||
| "sentry.integrations.tasks", | |||
| ) | |||
|
|
|||
| # tmp(michal): Default configuration for post_process* queueus split | |||
There was a problem hiding this comment.
Should this still be considered temporary?
| # tmp(michal): Default configuration for post_process* queueus split | |
| # tmp(michal): Default configuration for post_process* queues split |
There was a problem hiding this comment.
Yes, this will be replaced by CELERY_SPLIT_QUEUE_ROUTES below. I'd remove the comment when I remove the setting.
| # Use legacy route | ||
| # This router required to define the routing logic inside the | ||
| # settings file. | ||
| return settings.SENTRY_POST_PROCESS_QUEUE_SPLIT_ROUTER.get(queue, lambda: queue)() |
There was a problem hiding this comment.
This looks compatible with the patch that is in getsentry/ops config overrides 👍
| return ret | ||
|
|
||
|
|
||
| def make_split_queues(config: Mapping[str, SplitQueueSize]) -> Sequence[Queue]: |
There was a problem hiding this comment.
We don't seem to call this outside of tests. Do we need to setup the post_process_transactions split queues, or are we going to rely on the patching being done in getsentry/ops for a while longer?
There was a problem hiding this comment.
There was a debate in the previous PR whether we should add the queues in sentry or getsentry.
#76494 (comment).
What do you think?
I am a bit biased towards adding them in the sentry configuration rather than getsentry to create fewer surprises.
Either way, in order to rollout safely, I need to deploy this first, then adapt the current patch to avoid queue duplication (if you declare the same queue twice two queues actually show up), then I can switch to using this method.
tests/sentry/queue/test_routers.py
Outdated
| legacy_mode = options.get("celery_split_queue_legacy_mode") | ||
| options.set("celery_split_queue_legacy_mode", []) |
There was a problem hiding this comment.
You can use override_options to modify options values via a context manager similar to override_settings
There was a problem hiding this comment.
damn, that's much better, thanks.
#76410 introduces a way to deliver messages to a set of split celery queues rather than relying on a single queue for
post process.
This was used to address an incident as we were saturating Rabbit resources
for a single queue (which is single threaded). Splitting messages across multiple
queues solves that problem.
This PR introduce a configurable support for split queues for scenarios where we pass
the queue name to
apply_async.There is another PR to deal with tasks that define the queue in the task definition:
#76494
This introduces a router class SplitQueueRouter that maps a queue to a list of split
queues picked in a round robin way.
This router is used by the client code that schedules a task with apply_async.
The configuration is held in CELERY_SPLIT_QUEUE_ROUTES and all the
split queues have to be declared as any other queue.
Rollout procedure to remove the hack from #76410